Skip to content

Bug/fix inappropriate symbol and ! for factorial#255

Merged
m-messer merged 15 commits into
mainfrom
bug/fix-inappropriate-symbol-and-!-for-factorial
May 19, 2026
Merged

Bug/fix inappropriate symbol and ! for factorial#255
m-messer merged 15 commits into
mainfrom
bug/fix-inappropriate-symbol-and-!-for-factorial

Conversation

@m-messer
Copy link
Copy Markdown
Member

@m-messer m-messer commented May 13, 2026 •
edited
Loading

Requires #247 to be merged first before merging this PR.

Problem

Two bugs affecting infinity and factorial handling in symbolic expression comparison:

  1. The Unicode infinity symbol (∞) was not recognised as valid input, causing parse errors when students used it.
  2. Comparing infinite expressions (e.g. oo = oo) would subtract the two sides before checking, but oo - oo evaluates to nan in SymPy rather than 0, causing incorrect results. A KeyError/TypeError could also be thrown during the is_infinite check.
  3. The triple factorial notation !!! was silently mishandled — it parses as repeated ! (factorial of factorial of factorial) rather than the mathematical triple factorial, with no warning to the student.

Fix

  • Added ∞ as an alias for oo in expression_utilities.py so the Unicode symbol is accepted as input.
  • Added do_comparison_infinite in context/symbolic.py that compares infinite expressions directly (e.g. lhs == rhs) instead of via subtraction, and wrapped the is_infinite check in a try/except to guard against edge-case exceptions.
  • Added a NOTATION_WARNING_TRIPLE_FACTORIAL feedback message and wired it up in evaluation.py to warn students that !!! is not supported.

Other changes

  • Upgraded to Python 3.12, SymPy 1.14, and mpmath 1.4.1; updated CI workflows accordingly.
  • Fixed the Dockerfile: switched base image package manager, added find to installed requirements, set PYTHONPATH, and updated the CMD entrypoint format.

m-messer and others added 15 commits March 20, 2026 16:10
@m-messer
Updated to Python 3.12
255655d
@m-messer @claude
Handle infinity Unicode symbol and fix infinite value comparison
ccd621b 
Added ∞ as an alias for oo so students can use the Unicode infinity
symbol as input. Fixed comparison of infinite expressions by using
direct sympy comparison instead of subtraction, since oo - oo yields
nan rather than 0.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@m-messer
Handled exceptions for is_infinite checks during symbolic expression …
b442d0b 
…comparison
@m-messer
Merge branch 'main' into feature/python312
807556f
@m-messer
Updated sympy to 1.13
f3cb9bf
@m-messer
Updated sympy to 1.14 and mpath to 1.4.1
b5b4163
@m-messer
Added parametric factorial and double factorial tests; implemented wa…
10bc2fb 
…rning for unsupported triple factorial notation
@m-messer
Update CI to use Python 3.12
99ca088
@m-messer
Changed mpmath version
ebf8c73
@m-messer
Merge branch 'main' into feature/python312
02003e7
@m-messer
Switched Dockerfile base image to apt package manager for Git install…
aa6dcfe 
…ation.
@m-messer
Switched to use dnf
e583225
@m-messer
Added find to installed requirements
29775c9
@m-messer
Set PYTHONPATH in Dockerfile and updated CMD entrypoint format
26b2949
@m-messer
Merge branch 'feature/python312' into bug/fix-inappropriate-symbol-an…
4606471 
…d-!-for-factorial
@m-messer
Copy link
Copy Markdown
Member Author

m-messer commented May 13, 2026

Database testing can be found here: https://github.com/lambda-feedback/compareExpressions/actions/runs/25786405204/job/75740792212

7 errors out of 1000

  1. _Grade Mismatch - eval func True, DB False -- Answer 2(cos(pi/2)+isin(pi/2)), response 2( cos(π/2)+i sin(π/2))
  • Added support for unicode greek letters in a previous fix
  1. E^{x log x} ( log x + 1) could not be parsed as a valid mathematical expression. Ensure that correct codes for input symbols are used, correct notation is used, that the expression is unambiguous and that all parentheses are closed.
    • Answer written in LaTeX not Sympy
  2. 2x \ sin x + x^{2} \ cos x could not be parsed as a valid mathematical expression. Ensure that correct codes for input symbols are used, correct notation is used, that the expression is unambiguous and that all parentheses are closed.
  • Same as 2.
  1. x E^{x}(2+x) could not be parsed as a valid mathematical expression. Ensure that correct codes for input symbols are used, correct notation is used, that the expression is unambiguous and that all parentheses are closed.
  • Same as 2.
  1. asec**2(ax) could not be parsed as a valid mathematical expression. Ensure that correct codes for input symbols are used, correct notation is used, that the expression is unambiguous and that all parentheses are closed.
  • Invalid form for asec^2(ax), should be asec(ax)**2
  1. _Grade Mismatch - eval func True, DB False -- Answer -5 muA, response -5 µA
  • Added support for unicode greek letters in a previous fix
  1. (x cosx - sinx) / (x^{2}) could not be parsed as a valid mathematical expression. Ensure that correct codes for input symbols are used, correct notation is used, that the expression is unambiguous and that all parentheses are closed.
  • Same as 2.

@m-messer m-messer requested a review from peterbjohnson May 15, 2026 14:49
peterbjohnson
peterbjohnson approved these changes May 18, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@peterbjohnson peterbjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All ok with me and I've added an inline comment from Phil on the comparison of infinities. We'll do it and see how it goes.

Comment thread app/context/symbolic.py
def do_comparison_infinite(comparison_symbol, lhs_expr, rhs_expr):
# When either side is infinite, subtracting (e.g. oo - oo) yields nan, making
# the subtraction-based do_comparison unreliable. Compare the sides directly instead.
direct_comparisons = {
Copy link
Copy Markdown
Member

@peterbjohnson peterbjohnson May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment from Phil Ramsden after I asked him about this:

"I'm not speaking here as a mathematician exactly, but as someone who's written this kind of code for METRIC back in the day! The rationale for comparing the difference to zero is that zero is unambiguously "simpler" than almost any expression equivalent to it, and is therefore a likely end point for any chain of simplification. By contrast, it's possible to imagine (and not all that hard to concoct) examples of expressions that are algebraically equivalent but don't simplify to the same thing.

However, there are, potentially, several pitfalls with "compare difference to zero". Infinity is one of them, as there's no well-defined difference operation for infinities. A slightly different class of issue arises where the standard difference operation on equivalent objects exists all right, but yields something other than zero, such as the zero vector or (perhaps, depending on how difference is set up) the empty set. So "compare difference to zero" is not a panacea, and should only really be used when comparing symbolic expressions that represent scalars of some sort."

@m-messer m-messer merged commit ff7ca73 into main May 19, 2026
10 checks passed
@m-messer m-messer deleted the bug/fix-inappropriate-symbol-and-!-for-factorial branch May 19, 2026 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peterbjohnson peterbjohnson peterbjohnson approved these changes

Assignees

@peterbjohnson peterbjohnson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@m-messer @peterbjohnson